Performance: Cache config#276
Conversation
~22% fewer allocations (161 -> 125) ~14% fewer bytes used (9,936 -> 8,496) +18.7% iteration speed (23,114 ips -> 27,438 ips, 43.3μs -> 36.5μs per render) Using benchmarks/view_benchmark.rb on Ruby 4 (on an M1 Pro)
|
Thanks for looking into this stuff, @cllns! My question is whether we might actually be able to solve this at a Dry Configurable level, rather than create a workaround for it here? We use Dry Configurable in many other places, so a solve there once would benefit us more. |
cllns
force-pushed
the
performance/cache-config
branch
from
821a86e to
4c064ed
Compare
|
@timriley I was already working on it :) Just had to figure out the right API. See: dry-rb/dry-configurable#167 and updated changes here. I made a few tiny refactors to move things into private visibility. There's more I want to do, but I'll do that as a follow-on, so this PR doesn't get too complicated. |
Since dry-rb/dry-configurable#167 has been merged
|
Switched the Gemfile to use dry-configurable from 'main' since dry-rb/dry-configurable#167 has been merged |
|
|
||
| private | ||
|
|
||
| attr_reader :config_data, :prefixes |
There was a problem hiding this comment.
Just a note on why these were public before: I've never really cared about leaving public attr readers on objects, even if they're not formally "public API". It's Ruby, so if anyone really wanted to get to some object, there's always a way they can do it, so I never really tried to fight it.
This is not to say I'm opposed to this change, but I thought I'd just share some of the rationale I held when putting these things together in the first place :)
Happy for this change to go in as part of this PR, anyway :)
| # @since 2.3.0 | ||
| attr_reader :part_class, :part_namespace, :scope_class, :scope_namespace | ||
|
|
||
| # Stable identity for the underlying config snapshot, suitable as a cache-key component. |
There was a problem hiding this comment.
| # Stable identity for the underlying config snapshot, suitable as a cache-key component. | |
| # Stable identity for the underlying config snapshot |
Since this is already called cache_key, we probably don't need to repeat ourselves in the description :)
| # @api private | ||
| # @since 2.3.0 |
There was a problem hiding this comment.
Let's stop putting @since tags on new @api private methods. These are for us to use internally only, and putting a @since IMO implies some level of stability to external readers of the code that I don't want to promise. (It's private API, after all!)
I need to make a post about this, but in the meantime I'm trying to stop it proliferating, and removing the tags whenever I edit a file.
There was a problem hiding this comment.
(Also, for any "since" that is legitimately needed here — now for "@api public" methods only! — it should be @since 3.0.0)
... though I think most of your changes are to private API, AFAICT
|
|
||
| private | ||
|
|
||
| attr_reader :renderer |
There was a problem hiding this comment.
I mean, this is probably why I don't like private attr_readers. As a maintainer looking at this class, I see a bunch at the top of the file, and then we have this random one hanging out all the way at the bottom. It's disjointed and it's hard to get a good overview of the class this way.
Maybe we could just leave them at the top, since attr privacy is not really the purpose of this PR?
| output = rendering.template( | ||
| self.class.layout_path(layout), | ||
| rendering.scope(config.scope, layout_locals(locals)) | ||
| File.join(*[config_data.layouts_dir, layout].compact), |
There was a problem hiding this comment.
Could we leave the layout_path method back as it was? I think it makes this code here (which is already doing a lot!) easier to follow. One fewer piece of detailed implementation inlined into the method.
| # | ||
| # @since 2.3.0 |
There was a problem hiding this comment.
| # | |
| # @since 2.3.0 |
Another since we can drop. This is not just "@api private", it's fully private :)
| gem "stackprof", platform: :mri | ||
| gem "memory_profiler" |
There was a problem hiding this comment.
Are these not strictly needed for this PR?
2 participants
~28% fewer allocations (138 -> 99) per render
~18% fewer bytes used (8,739 -> 7,160) per render
+36.8% iteration speed (30,891 ips -> 42,266 ips, 32.4μs -> 23.7μs per render)
(Updated these numbers. The early improvements weren't as high but I closed some apps 😇 and these seem to be consistent)
Using benchmarks/view_benchmark.rb on Ruby 4 (on an M1 Pro).
I originally had this with a class-level
self.cached_configwhich isn't necessary so I removed it. This means each instantiation of a view has to computecached_configfor itself, adding some overhead. Since we've built this library based on the assumption that a single View will be re-used (rather than created new each time it's used), I think that's fine. Open to other perspectives though!Another option is using
reader: truefor all of the settings. I experimented with this too, but it's not as clean, because the settings are set on theViewclass rather than the config, soview_classhas to be passed around everywhere, which hides the intent of sending just config settings around. It's got basically the same performance as this approach though.